Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix create balance when incomplete data #315

Merged

Conversation

jauggy
Copy link
Member

@jauggy jauggy commented Jun 8, 2024

Fixes #314

Unit Test Steps

mix test --only balance_test

Test Steps

Run local browser. Login as admin. Go to Admin > Matches > Show a match > Go to balance tab
It should have no errors

@@ -4,30 +4,50 @@ defmodule Teiserver.Battle.SplitOneChevsInternalTest do
Can run tests in this file only by
mix test test/teiserver/battle/split_one_chevs_internal_test.exs
"""
use Teiserver.DataCase, async: true
use ExUnit.Case
@moduletag :balance_test
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to this file are just formatting and also since we don't hit the db, we use ExUnit instead

@jauggy jauggy marked this pull request as ready for review June 8, 2024 11:37
Copy link
Contributor

@geekingfrog geekingfrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix, left a few comments about general code quality, but the functionality looks good.


better_map
end)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this bit correctly, when the value in the map is not a user struct, you want to fetch the corresponding user and replace the id with that user right?

In that case there's a much simpler way: Enum.map

    Enum.map(groups, fn group ->
      group
      |> Enum.map(fn {user_id, value} ->
        cond do
          is_number(value) -> {user_id, get_user_rating_rank_old(user_id, value)}
          true -> {user_id, value}
        end
      end)
      |> Enum.into(%{})
    end)

or, if you prefer using guards, you can also do the following

    Enum.map(groups, fn group ->
      group
      |> Enum.map(fn
        {user_id, value} when is_number(value) ->
          {user_id, get_user_rating_rank_old(user_id, value)}

        {user_id, value} ->
          {user_id, value}
      end)
      |> Enum.into(%{})
    end)

The advantage of Enum.map is that the intent is clearer, you are applying some sort of transformation on every element. In the case of a map it's a tuple of {key, value}.
When using map_reduce, it's a lot more complicated to figure out what's going on. The fact that you had to discard one of the return value is a code smell as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a feeling there was a better way to do it. Before you reviewed this, I had already posted a question on Elixir forums: https://elixirforum.com/t/iterate-through-map-of-dynamic-keys-and-update-their-values/64061/3
I'm liking the idea of Map.new

%{3 => 18},
%{4 => 15},
%{5 => 11}
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a fan at all of the use of mocks. It forces you to have a deep knowledge of the internals of the stuff you're testing. You recognise that with the comment a bit bellow:

loser_picks algo will hit the databases so let's just test with split_one_chevs

It's also a departure from the other tests elsewhere.

For these reason, I'd rather have something that creates the mock data in the DB, and then exercise the code under test.

Using DataCase, async: true, I propose the following:

    [user1, user2, user3, user4, user5] =
      Enum.map(1..5, fn k ->
        Teiserver.TeiserverTestLib.new_user("User_#{k}")
      end)

    groups = [
      %{user1.id => 19, user2.id => 20},
      %{user3.id => 18},
      %{user4.id => 15},
      %{user5.id => 11}
    ]

    fixed_groups = BalanceLib.standardise_groups(groups)

    assert fixed_groups == [
             %{
               user1.id => %{name: "User_1", rank: 0, rating: 19},
               user2.id => %{name: "User_2", rank: 0, rating: 20}
             },
             %{user3.id => %{name: "User_3", rank: 0, rating: 18}},
             %{user4.id => %{name: "User_4", rank: 0, rating: 15}},
             %{user5.id => %{name: "User_5", rank: 0, rating: 11}}
           ]

and similar for the other test. You could even consider pulling the creation of users in a setup function for the module.

Let me know if you have any question over this code.

Copy link
Member Author

@jauggy jauggy Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this and any db changes are persisted between tests which means I cannot call TeiserverTestLib.new_user at beginning of each test. That function will create a user with a random name if it finds the user already exists.

It's really weird. If I call the test multiple times it will eventually fail.

"""
use ExUnit.Case
import Mock
@moduletag :balance_test
Copy link
Contributor

@geekingfrog geekingfrog Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced by this tag. Test tags should be used for logical grouping only. Like tag: cluster when you have several tests spanning multiple files testing one feature.
If you only want to test one file, you can always run mix test path/to/the/file

I think that tag could make sense for all balance related tests. But in this case, let's make another PR that tags all the relevant tests at once. And the tag should only be :balance since in this context, it's clear this is about testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other balance tests are tagged already though. So it's hitting multiple files.

@jauggy
Copy link
Member Author

jauggy commented Jun 8, 2024

@geekingfrog have updated the unit test to use the db instead of mocks. But if I try and run it multiple times it will fail. try

mix test path/to/balance_lib_internal_test.exs

Try running this multiple times and it should fail. I'm not sure how to fix. You can see example below

failure.mp4

@jauggy jauggy force-pushed the jauggy/fix-create-balance branch from 779bf62 to d7a6c3a Compare June 9, 2024 02:34
@StanczakDominik
Copy link
Collaborator

I've applied the mix format stuff from master :)

Copy link
Collaborator

@StanczakDominik StanczakDominik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I tested it on the lobbies that previously failed; the test failure on multiple runs is another issue, so I think it's fine to ignore until we fix it at the root cause. As long as the tests pass "once" (which is why having CI that we can take as mostly-a-source-of-truth is helpful) after dropping the database, I'm fine with this.

Thank you! :)

mix.lock Outdated Show resolved Hide resolved
@StanczakDominik StanczakDominik merged commit f49619c into beyond-all-reason:master Jun 9, 2024
1 of 3 checks passed
@jauggy
Copy link
Member Author

jauggy commented Jun 9, 2024

Have added #317
so that someone can fix the tests not clearing. I couldn't figure it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

/teiserver/admin/matches/:match_id fails, while /battle/:match_id/ratings works, after #235
3 participants